Refactored retry config into _retry.py and added support for exponential backoff and Retry-After header
#871
Conversation
| class HttpxRetry: | ||
| """HTTPX based retry config""" | ||
| # TODO: Decide | ||
| # urllib3.Retry ignores the status_forcelist when respecting Retry-After header |
There was a problem hiding this comment.
Does this mean urllib3.Retry retries other status codes (that's not in status_forcelist) with Retry-After headers?
| return False | ||
|
|
||
| # Placeholder for exception retrying | ||
| def is_retryable_error(self, error: Exception): |
There was a problem hiding this comment.
It is best not to leave placeholders in PRs if we can. If this is currently a no-op we can remove it and add it when we need it
| self.history.append((request, response, error)) | ||
|
|
||
|
|
||
| # TODO: Remove comments |
There was a problem hiding this comment.
You can remove these. Or add a comment on what it doesn't cover as a TODO for future. I prefer we use a bug/issue to track these instead of TODO comments when possible
|
|
||
| def __init__( | ||
| self, | ||
| status: int = 10, |
There was a problem hiding this comment.
Would it make sense to call this max_retries instead?
| response, error = None, None | ||
|
|
||
| try: | ||
| logger.debug('Sending request in _dispatch_with_retry(): %r', request) |
There was a problem hiding this comment.
Do we plan to keep these logs in the production code?
| if error and not retry.is_retryable_error(error): | ||
| raise error | ||
|
|
||
| retry.increment(request, response) |
There was a problem hiding this comment.
Should we pass the error here? retry.increment(request, response, error)
| return response | ||
| if error: | ||
| raise error | ||
| raise Exception('_dispatch_with_retry() ended with no response or exception') |
There was a problem hiding this comment.
Let's narrow this down to an Exception type RuntimeError or AssertionError might work better here.
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Is there a way to set the log level in production code?
| # Simulate attempt 6 completed | ||
| retry.increment(self._TEST_REQUEST, response) | ||
| # History len 6, attempt 7 -> 0.5*(2^4) = 10.0 | ||
| assert retry.get_backoff_time() == pytest.approx(10.0) |
There was a problem hiding this comment.
Should this be 0.5 * (2^5) = 16.0? and clammed at 10 due to backoff_max? If that's the case let's clarify it here :)
| self.status_forcelist = status_forcelist | ||
| self.backoff_factor = backoff_factor | ||
| self.backoff_max = backoff_max | ||
| self.raise_on_status = raise_on_status |
There was a problem hiding this comment.
If raise_on_status is unused we should just remove it
Adds more retry logic support
This PR covers:
HttpxRetryTransportto_retry.pyHttpxRetryto manage retry stateHttpxRetryTransportcreationRetry-Afterheaders from request responsesHttpxRetryandHttpxRetryTransport